This file contains #include <QColor>, which causes the CI_Without_Qt check to fail.

That's a clue that this is the wrong approach here. You shouldn't need to use Qt functionality in the engraving module.

One solution might be to return an MStyle object directly, instead of returning a path to a style file. That would enable you to call DefaultStyle::defaultStyle().

Otherwise, you'll need another way to get the file path to the default style.

Probably your getStyleFilePath() function should behave more like DefaultStyle::resolveStyleDefaults().

The first time DefaultStyle::resolveStyleDefaults() is called with a certain argument, it creates an MStyle object from a style file. If it gets called again in future with the same argument, it just returns the same MStyle object it created on the first call. So the actual style files only get accessed once.

Another possibility is to use IEngravingConfiguation instead of INotationConfiguration. IEngravingConfiguration namely also has a defaultStyleFilePath method. (In fact, the one from INotationConfiguration uses IEngravingConfiguration.)

In general, you shouldn't really ever include notation files in the engraving module. This is to avoid circular dependencies. Instead, the dependencies between modules should be seen as a hierarchy, and engraving is near the top of that hierarchy, meaning that it should not depend on most modules.

I see, yes that makes sense. Let me try it out

PropertyItem* properties are supposed to be constant:

Why? The PropertyItem* is not the value of the property, but an object that has own properties for the value, default value, etc.
So, the properties of that object can change and can be modified from QML, so those have NOTIFY and WRITE functions. But that object itself stays constant.

I'm starting to doubt more and more whether this should be a "property" at all.

The first question is: of what EngravingObject is it a property? Surely not of any individual element, but perhaps of a Score or MasterScore. But these classes don't work with properties at all currently, and thus their properties are also not written to a file.

Instead of with properties, Score and MasterScore work with "style" items. So, should this option be a style option? Perhaps, but also not really: that results in the weird situation that one special style item determines which style file needs to be loaded, and that causes all other style items (and if you don't watch out, even that special item itself) to be overridden.

Judging from how you are currently using this property, namely only in ScoreAccessibilitySettingsModel, it should be neither a property, nor a style item, because its value isn't stored anywhere anyway.

Just noticed my reply was still 'pending'. Looks like I forgot to click the 'submit' button!

I don't think we were fully clear about what we were expecting here, partly because we weren't completely sure ourselves.

For the MVP, the style preset dropdown can be purely a UI concept; it doesn't have to exist in the score model/file at all. When the dropdown value changes, a new style would be loaded like with Format > Load Style. MuseScore would instantly "forget" that the settings came from a file rather than manually via Format > Style.

However, I think the eventual aim is to save the name of the MSN style in the score, probably as a style rather than a property, or maybe even as an attribute on the <Style> element:

<Score>
    <Style preset="16mm MSN">
        <titleFontFace>Arial</titleFontFace>
        <Spatium>4</Spatium>
        <!-- more style settings -->
    </Style>
</Score>

If this attribute is present, that does not mean we will ignore the individual style settings in the score and load the specified MSN style file again and again whenever the score is opened.

Instead, the individual style settings will take priority for engraving purposes. The preset attribute would only be used to set the UI dropdown to the correct value when the score is opened.

Once we know what value to use in the dropdown, we can potentially do something more clever with it in a future version. For example, if the user clicks "Reset all styles to default", we could reset to the specified MSN default rather than to the global default. Or we can show a dialog like this:

Load the new 16mm MSN style?
This score was created in MuseScore Studio 4.5 using an old version of the 16mm MSN style. Would you like to load the new version of this style?
[ Yes ]          [ No ]

However, anything beyond simply storing the preset name and using it to determine the dropdown value is outside the scope of this GSoC project.

Thanks @shoogle @cbjeukendrup for providing the insights. I just updated the PR, will further refine it as we continue this week

Because the Pid::SCORE_STYLE_PRESET property is not a normal property of an element, the usage of PropertyItem* is not appropriate here. Instead, a simple dedicated getter and setter method should be created.

The unusual use of PropertyItem* also leads to a very complicated code flow here. So, when the user clicks an option in the dropdown, this code is called...

...which leads to here...

...and then this signal...

...causes this handler to be called, which updates the model of the dropdown.

In total, that's a very indirect way of updating the model of the dropdown.

A better way would be to make possibleScoreStylePresets a Q_PROPERTY with a NOTIFY signal, instead of a Q_INVOKABLE. When doing so, you need to drop the () here in QML, and the Connections object should be removed too. All you need to do is emitting the NOTIFY signal from C++ at the appropriate moment.

Yes Casper makes sense :D . Thank you for providing such a thorough insight, helpful ✨

If you want to save the edited status, you should do this in a separate attribute:

<Style preset="16mm MSN" edited="true"/>

Combining them as "16mm MSN (edited)" creates a microsyntax within XML that's extra work to read and write.

It would be better to check the current style values against the MSN style file to see if the current values were edited, but storing the "edited" attribute is an acceptable workaround for now.

This function is not used in the QML so it should be private and take an enum as argument rather than an int.

Use #pragma once instead of include guards in all new files.

@cbjeukendrup, what do you think of moving this enum to types.h as enum ScoreStylePreset?

That way it could have userName(), translatedUserName() and maybe toXml() fromXml() functions in typesconv.h.

That could be a good idea indeed.
(Perhaps in the further future it won't be an enum anymore but for example a string or preset filename; but for now this enum, with accompanying TConv stuff as far as necessary, is fine.)

So we port the enum?

@nasehim7, yes, please move it to types.h as enum ScoreStylePreset and define userName() and translatedUserName() functions for it.

You can then use the translatedUserName() function in ScoreAccessibilitySettingsModel::possibleScoreStylePreset(), when you create the QVariantList.

Instead of writing the QVariantList out by hand, you can iterate over the enum (you'll need a dummy ScoreStylePreset::MAX_PRESET value to stop iteration). For each enum value, you can get its text from the translatedUserName() function and insert it into the list. You can also test to see whether it is the current preset and is edited. If so, you can insert the %1 (edited) value into the list straightaway. This is better than doing it at the end because it saves having to make space for it in the list by shuffling all the other items down a place.

Okay sure Peter! let me do that and update the PR

@shoogle Updated the PR with the suggested approach

This should be double layer now:

QVariantList presets = {
        QVariantMap {
            { text, muse::qtrc("inspector", "Default") },
            {
                value,
                QVariantMap {
                    { preset, static_cast<int>(AccessibilityStylePreset::DEFAULT) },
                    { edited, false }
                }
            },
        },
        QVariantMap {
            { text, muse::qtrc("inspector", "16mm MSN") },
            {
                value,
                QVariantMap {
                    { preset, static_cast<int>(AccessibilityStylePreset::MSN_16MM) },
                    { edited, false }
                }
            },
        },
        QVariantMap {
            { text, muse::qtrc("inspector", "18mm MSN") },
            {
                value,
                QVariantMap {
                    { preset, static_cast<int>(AccessibilityStylePreset::MSN_18MM) },
                    { edited, false }
                }
            },
        },

I'd still set QString text = "text", QString value = "value", etc. outside the loop, then use them here inside the loop, like this:

presets.append(QVariantMap {
    { text, presetName },
    {
        value,
        QVariantMap {
            { preset, i },
            { edited, false }
        }
    }
});

Then I would write out the edited one explicitly like this as well.

Previously I asked you to recycle the map object, but that's because you had to unpack it anyway to get the string and the integer value. That's not the case anymore, so you might as well create a new one.

This function should return the value directly rather than an index to the value.

return QVariantMap {
    { "preset", style.stylePreset() },
    { "edited", style.stylePresetEdited() }
}

Then in ScoreAccessibilityInspectorView.qml you can do:

scoreStylePreset.currentIndex = indexOfValue(root.model.scoreStylePreset())

@shoogle I tried to do this but came across a finding that there is strict check in the indexOfValue() method. So as per JS Docs If both operands are objects, return true only if they refer to the same object. which is not the case here due to the fact that even though two objects have the same values, they are considered different objects in memory unless they are the exact same reference so it was going back to -1 index. I thought of multiple solutions but the one where we pass the model to the scoreStylePreset() sounded fair. Ideally, this method should now be called like getScoreStylePresetIndex() or something which would look like

QVariantMap ScoreAccessibilitySettingsModel::getScoreStylePresetIndex(const QVariantList& presets) const
{
    MStyle style = globalContext()->currentNotation()->elements()->msScore()->style();
    int currentPresetValue = static_cast<int>(style.stylePreset());

    QString text = "text";
    QString value = "value";
    QString preset = "preset";
    QString edited = "edited";

    for (const QVariant& p : presets) {
        QVariantMap presetMap = p.toMap();
        QVariantMap valueMap = presetMap.value(value).toMap();
        int presetValue = valueMap.value(preset).toInt();
        bool editedValue = valueMap.value(edited).toBool();

        if (presetValue == currentPresetValue && editedValue == style.stylePresetEdited()) {
            return p.toMap().value(value).toMap();
        }
    }
    
    return presets.first().toMap().value(value).toMap(); // or we could do: return QVariantMap();
}

or Am I missing something? I checked when I do JSON.stringify() in the JS file, it works

I think it's okay for this method to return int, but would rename it to currentScoreStylePresetIndex. However the implementation can be cleaned up; casting to int and then checking every possible int value is not great. Either rewrite it as a switch, without a cast to int; or as an arithmetic expression, i.e. return isAccessibilityStyleEdited ? presetIndex + 1 : presetIndex. Personally I'd prefer the latter in this case.

Ahh, good catch! I actually had a go at this myself and wondered why it wasn't working... 🤪

There are three options here:

1. Make the check non-strict (i.e. use == instead of ===).
   • Qt Creator will display a warning if you do this, so you'd need to leave a comment there so that nobody "fixes" it later on.
2. Unpack the possibleScoreStylePresets() list and return the actual object rather than an equivalent one.
   • I don't think this would work though, because you'd get a different (albeit equivalent) object each time you call the function.
   • If it happens to always give the same object on your machine, that might be a compiler optimization that wouldn't necessarily work with other compilers or with other compiler settings.
3. Return an index instead of a value.

Your suggested code is a strange mix of 2 and 3. You rename the function to include "index", but then you return an object (i.e. a value) from the list.

Perhaps it is best to return an index after all. If so, this is the way to do it:

int ScoreAccessibilitySettingsModel::scoreStylePresetIndex() const
{
    MStyle style = globalContext()->currentNotation()->elements()->msScore()->style();
    int currentPresetValue = static_cast<int>(style.stylePreset());
    if (style.stylePresetEdited()) {
        return currentPresetValue + 1;
    }
    return currentPresetValue;
}

This works because when you looped over the enum values to create the QVariantList in possibleScoreStylePresets(), you already assumed that the first enum value is 0 and it increments by 1 each time.

If that assumption is true (which we know it is from types.h) then it follows that the index in the list is equal to the enum value. If it's edited you just need to add 1 to that value.

The presence of the edited value in the list throws all the later indexes off by one. But the neat thing is that the edited one is removed from the list as soon as it is no longer the current one, so this function never needs to return an index "after" the edited one.

TLDR: Pick option 3 and use that code snippet. It will be faster than the other options because it avoids iterating over the list. You can use your knowledge of the enum to jump straight to the correct index.

Missing 's' at end of function name: possibleScoreStylePresets().

This function is no longer needed. You can use untranslated the TConv version instead.

You can use the untranslated TConv function here.

Under INJECT and above public, you should have these lines:

Q_PROPERTY(QVariantList possibleScoreStylePresets READ possibleScoreStylePresets NOTIFY possibleScoreStylePresetsChanged);
Q_PROPERTY(int scoreStylePresetIndex READ scoreStylePresetIndex WRITE setScoreStylePresetIndex NOTIFY scoreStylePresetIndexChanged);

This declares two properties that you can use in the QML as:

• model.possibleScoreStylePresets
• model.scoreStylePresetIndex

For this to work, the class also needs to have these functions, as declared in the Q_PROPERTY() macros:

public:
    QVariantList possibleScoreStylePreset() const; // READ: not Q_INVOKABLE now
    int scoreStylePresetIndex() const;             // READ: not Q_INVOKABLE now
    int setScoreStylePresetIndex(int index);       // WRITE: replaces loadStyle(int preset)

signals:
    void possibleScoreStylePresetsChanged(); // NOTIFY: replaces accessibilityStyleEditedChanged()
    void scoreStylePresetIndexChanged();     // NOTIFY

Note: There's no WRITE function for possibleScoreStylePreset because it's a read-only property.

Now, assuming you're emitting (with emit) those signals in the right places, if you set this in the QML:

StyledDropdown {
    id: scoreStylePreset
    currentIndex: model.scoreStylePresetIndex
    // Etc.
}

Qt will use the READ and NOTIFY functions specified in the Q_PROPERTY() macro to make sure that scoreStylePreset.currentIndex always stays in sync with model.scoreStylePresetIndex.

Ah yes makes sense and it is much more cleaner now. Thanks Peter and Casper for the inputs :D Just updated the PR with the changes

onActivated: function(index, value) {
    root.model.scoreStylePresetIndex = index // internally, Qt will call the WRITE function
                                             // specified within the Q_PROPERTY() macro.
                                             // i.e. setScoreStylePresetIndex(index)
    if (!value.edited) {
        root.model.loadStyle(value.preset)
    }
}

You could remove the call to root.model.loadStyle(value.preset) and do that inside setScoreStylePresetIndex(index) instead. But it's fine to do it here as long as you don't do it in both places.

I think it might be preferable to call loadStyle from C++ indeed. If you only change the index, but don't call loadStyle, things are in an inconsistent state, so you'll never want to update only the index. Therefore, always calling loadStyle from C++ seems good to me.

Yeah makes sense. Moved the logic to the setScoreStylePresetIndex(index)

There should either be three variables here:

ScoreStylePreset m_scoreStylePreset = m_scoreStylePreset::DEFAULT;
bool m_scoreStylePresetEdited = false;
bool m_ignoreStyleChange = false;

Or there should just be m_ignoreStyleChange.

If you're caching the edited value, you might as well cache the stylePreset value as well.

If you have these values cached, you might as well use them everywhere in the .cpp instead of reading from the style each time.

I see. I have updated the code with the caching approach

Is this function still needed? If so, you should change it as follows:

void ScoreAccessibilitySettingsModel::updateScoreStylePreset()
{
    MStyle style = globalContext()->currentNotation()->elements()->msScore()->score()->style();
    ScoreStylePreset stylePreset = style.stylePreset();
    bool stylePresetEdited = style.stylePresetEdited();
    bool indexChanged = false;

    if (m_scoreStylePresetEdited != stylePresetEdited) {
        m_scoreStylePresetEdited = stylePresetEdited;
        emit possibleScoreStylePresetsChanged();
        indexChanged = true;
    }

    if (m_scoreStylePreset != stylePreset) {
        m_scoreStylePreset = stylePreset;
        indexChanged = true;
    }

    if (indexChanged) {
        emit scoreStylePresetIndexChanged();
    }
}

Your setScoreStylePresetIndex(int index) should look a little bit like this, though not exactly the same.

I think it's needed as when the style file is read, the same values need to be propagated to the scoreaccessibilitysettingsmodel member variables (m_scoreStylePresetEdited, m_scoreStylePreset). This should happen when the UI component load is completed so under Component.OnCompleted: {}.

Here you're supposed to use the supplied int index to determine whether the preset enum and/or edited value should change. If so, you should update those values in the style (as well as your cached versions if caching) and then emit the appropriate signals.

Yes, that makes sense. I thought about it like this:

As we know, the member variables m_scoreStylePreset and m_scoreStylePresetEdited are caching the latest ScoreStylePreset and whether it is an edited version or not. We can use this information to determine if there has been a change. When m_scoreStylePresetEdited is false, it means there are 6 values in the dropdown; otherwise, there are 7 values, with the edited option always appearing just below the corresponding standard option. So it should look like the following:

void ScoreAccessibilitySettingsModel::setScoreStylePresetIndex(int index)
{
    bool isEdited = false;
    ScoreStylePreset selectedPreset;

    if (m_scoreStylePresetEdited) {
        if (index == static_cast<int>(m_scoreStylePreset) + 1) {
            isEdited = true;
        }
        selectedPreset = static_cast<ScoreStylePreset>(index - (isEdited ? 1 : 0));
    } else {
        selectedPreset = static_cast<ScoreStylePreset>(index);
    }

    bool presetChanged = (selectedPreset != m_scoreStylePreset);
    bool editedChanged = (isEdited != m_scoreStylePresetEdited);

    if (presetChanged || editedChanged) {
        m_scoreStylePreset = selectedPreset;
        m_scoreStylePresetEdited = isEdited;

        if (!isEdited) {
            loadStyle(static_cast<int>(m_scoreStylePreset));
        }

        emit possibleScoreStylePresetsChanged();
        emit scoreStylePresetIndexChanged();
    }
}

I think the signal possibleScoreStylePresetsChanged() should ideally be emitted when there is a selection from edited version to standard and not from standard to standard but yeah for simplicity, I have kept it like this for now. Let me know if this is appropriate, Peter! I just updated the PR :)

To save yourself the hassle of fixing all the unit tests, you can avoid storing the preset if it's the default one.

Also, you don't want to call .translated() on these. It's best to stick to the source language for semantic text, otherwise you have to handle every possible language in your read functions. You should use (i.e. create) a TConv::toXml() function. It will return AsciiStringView rather than TranslatableString, so you can't accidently call .translated() on it.

I see, updated it in the latest push. Ran the tests locally and it was working fine. Thanks for the suggestion :)

I added the logic to skip saving the default preset like following:

if (presetEdited()) {
        if (preset() != ScoreStylePreset::DEFAULT) {
            xml.startElement("Style", {
                { "preset", TConv::toXml(preset()) },
                { "edited", String(u"true") }
            });
        } else {
            xml.startElement("Style", {
                { "edited", String(u"true") }
            });
        }
    } else if (preset() != ScoreStylePreset::DEFAULT) {
        xml.startElement("Style", {
            { "preset", TConv::toXml(preset()) }
        });
    } else {
        xml.startElement("Style", {});
    }

Currently, If the score style preset is default but is edited, at the time of saving, I am only adding the edited tag only to the XML. At the time of reading, if the preset tag is not present, the code sets the preset to default.

Better to remove this function and create a fromXml() function in typesconv.cpp instead.

Added